-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exposure Analysis Download CSV access from anywhere #920
Exposure Analysis Download CSV access from anywhere #920
Conversation
Build succeeded and deployed at https://prism-920.surge.sh |
Nice work on this @HarryMytilinaios! This is very much needed. We need to harmonize this with the UI that @hafidz142 has worked in PR #919. I mentioned the same to @ericboucher. Thanks for syncing up on this with them next week |
…m-multiple-places
@HarryMytilinaios - I sync'd with @ericboucher about this earlier. Let's go ahead and merge this and we'll have @hafidz142 update his PR afterwards Pending @ericboucher's approval! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ This pull request was sent to the PullRequest network.
@HarryMytilinaios you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PullRequest Breakdown
Reviewable lines of change
+ 261
- 101
72% TSX
26% TypeScript
2% Jest Snapshot (tests)
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this set of changes is looking good, nice work! I've left one small recommendation that could optionally be applied before merging. No major issues stood out.
Reviewed with ❤️ by PullRequest
const analysisDefinition = useSelector(getCurrentDefinition); | ||
|
||
const exposureAnalysisTableData = getExposureAnalysisTableData( | ||
analysisData?.tableData as TableRow[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optional chain implies that this expression might return undefined
, but I see it's being cast as an always-truthy TableRow[]
. Could a fallback empty array make this line less error-prone?
(analysisData?.tableData || []) as TableRow[],
A similar suggestion applies to the related changes in AnalysisDownloadButton.tsx
in this PR.
🔹 Reduce Future Bugs (Nice to have)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No merge blocking issues found. Changes are in line with what is stated in the description of the PR. Thanks for the pull request.
Reviewed with ❤️ by PullRequest
This PR adds the
Download CSV
functionality for theExposure Analysis
both in theLayer Accordion
component and in theLegend
component, giving us access to download it from both places as well as the theExposureAnalysisActions
below the table. The sorting functionality is also ported to all places.